Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cli): added Ctrl+Z (SIGTSTP) support #2836

Closed
wants to merge 1 commit into from

Conversation

Andrew15-5
Copy link
Contributor

Fixes one entry from #2788 (comment) by adding support for Ctrl+Z. I did face a few problems. But overall it works.

It adds (signal) to the zsh's suspend job log message, which isn't there when SIGTSTP is used in other programs. Another thing is that when it is stopped in the middle of some building process (at least during Wasm shenanigans) it can ruin the build (with a panic message) and the dx serve must be restarted. This should be explored more because suspending shouldn't break anything ideally.

The quirk that is interesting is where the execution is continued. This is nice to know as I used a condition check in the main loop before and didn't understand why no logs from it showed up. The less code, the cleaner it gets!

It looks like there is only 1 new package according to Cargo.lock. Nice.

I added a few notes in different places. Like using private field from screen variable isn't great.

@Andrew15-5
Copy link
Contributor Author

Andrew15-5 commented Aug 16, 2024

I finally remembered to test it from just recipe. Yeah, it doesn't work like that. Or rather, I have to press Ctrl+Z twice. I think, once to suspend dx and once to suspend just. So I'm not sure if it's OK to use this implementation of the feature. It's definitely better than nothing, but it isn't perfect (it's very close though).

I can try another approach, which basically means remove the root cause instead of making a workaround.

@Andrew15-5
Copy link
Contributor Author

@jkelleyrtp, ready for initial review, thoughts, verdict. Still has some debugging stuff.

@jkelleyrtp
Copy link
Member

Hey - I'm cleaning up the PR list and this got clobbered by a bunch of changes we made regarding the CLI.

Feel free to re-open this if you want. Definitely give the new CLI a test drive first though, since this might be fixed with the new render code.

@jkelleyrtp jkelleyrtp closed this Oct 26, 2024
@Andrew15-5
Copy link
Contributor Author

Feel free to re-open this if you want. Definitely give the new CLI a test drive first though, since this might be fixed with the new render code.

Sure! But did you know that a regular user can't re-open if someone with write permissions/member/owner has closed the PR (or issue)? So this means that you need to re-open it for me if I want it.

@Andrew15-5
Copy link
Contributor Author

I tried the new CLI, and it still doesn't support terminal stop signal. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants